Vulkan: take im2col when the gather is cheap, not only when c_out is large - #22782
Open
msluszniak wants to merge 2 commits into
Open
msluszniak wants to merge 2 commits into
msluszniak wants to merge 2 commits into
Conversation
pytorch-bot
Bot
added
the
module: vulkan
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22782
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 1 Awaiting Approval, 1 New Failure, 1 Unclassified FailureAs of commit 3f13ed9 with merge base 903696a ( NEW FAILURE - The following job has failed:
UNCLASSIFIED FAILURE - DrCI could not classify the following job because the workflow did not run on the merge base. The failure may be pre-existing on trunk or introduced by this PR:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
msluszniak
changed the title
msluszniak
force-pushed
the
ms/vulkan-im2col-mali-threshold
branch
from
September 13, 2026 14:15
520b359 to
2baedd2
Compare
msluszniak
changed the title
Third iteration of this PR. The c_out gate regressed fast_sam 18%, and bounding the gather regressed fcn_resnet50 by 19% (2938 -> 3505 ms): fcn's head is a 2048->512 3x3 at 65x65, which wants a 77.9M element gather but reads it back 512 times, so it is exactly the case im2col exists for. Neither variable works alone. They are independent reasons. A large gather is fine when c_out is large, because the GEMM amortizes it c_out times. When c_out is small it is only worth paying if the matrix is small outright, which is where the direct shader on Mali loses badly. So keep upstream's c_out rule and add a cheap-gather disjunct, scoped to Mali so no other device changes behaviour. Measured on a Mali-G76 (Galaxy S10+), fp16, interleaved GPU timestamps, median. 'before' is upstream today: | model | before | c_out gate | gather bound | this | | --- | --- | --- | --- | --- | | style_transfer_candy @640 | 1912.15 | 1436.31 | 1410.94 | **1411.67** | | fast_sam s @640 | 345.09 | 404.54 | n/a | **341.61** | | fcn_resnet50 @520 | 2938.13 | 2993.47 | 3505.06 | **2948.10** | fcn gives up 0.3%, which is at the edge of the run-to-run range. The other two are wins and neither of the earlier rules was a win on all three. Regression check (these runners also carry the 5x5 depthwise tile fix, which is why the mobilenet models move): | model | before | this | | --- | --- | --- | | lraspp @520 | 71.28 | 67.78 | | deeplabv3-mnv3 @520 | 165.27 | 161.76 | | selfie @256 | 6.49 | 6.18 | | ssdlite320 | 37.79 | 35.80 |
msluszniak
force-pushed
the
ms/vulkan-im2col-mali-threshold
branch
from
September 13, 2026 21:22
2baedd2 to
90f63a4
Compare
executorch-triage
Bot
added
the
community: contribution
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated twice; this is the third rule. Both earlier versions regressed a model, so the history is worth stating:
c_outgate regressed fast_sam on Mali by 18% (GPU 345 -> 405 ms). Its 80x80 convolutions havec_out64, so the gate pushed them onto the direct shader.Neither variable works alone because they are independent reasons to take im2col. A large gather is fine when
c_outis large, since the GEMM amortizes itc_outtimes. Whenc_outis small it is only worth paying if the matrix is small outright, which is where the direct shader on Mali loses badly.So this keeps upstream's
c_outrule and adds a cheap-gather disjunct, scoped to Mali so no other device changes behaviour.Mali-G76 (Galaxy S10+), fp16, interleaved GPU timestamps, median. "before" is upstream today:
fcn gives up 0.3%, at the edge of the run-to-run range. The other two are wins, and neither earlier rule was a win on all three.
Regression check (these runners also carry #22784, which is why the mobilenet models move):
32M is calibrated on this device, not derived. The cases it separates are far apart (~4M and ~30M on one side, ~100M and up on the other), so the exact value is not delicate, but a maintainer with other Mali parts may want to revisit it. Adreno is deliberately untouched.
cc @SS-JIA @manuelcandales @digantdesai @cbilgin